Skip to content

Use typed UUIDs for silo user and group #8803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jmpesp
Copy link
Contributor

@jmpesp jmpesp commented Aug 8, 2025

This commit changes SiloUser and SiloGroup to use typed UUIDs.

The biggest reason that this couldn't happen without a bunch of work was the lookup_resource macro: for a resource like SshKey, it looked like

lookup_resource! {
    name = "SshKey",
    ancestors = [ "Silo", "SiloUser" ],
    lookup_by_name = true,
    soft_deletes = true,
    primary_key_columns = [ { column_name = "id", rust_type = Uuid } ]
}

One of the methods that the lookup_resource macro generates will create a lookup for ancestors of the resource, and this was one using the type returned from the corresponding authz resource's id method:

            quote! { .filter(dsl::#parent_id.eq(#parent_authz_name.id())) },

Changing SiloUser to use a typed UUID in the authz resource:

authz_resource! {
    name = "SiloUser",
    parent = "Silo",
    primary_key = { uuid_kind = SiloUserKind }, <-- here
    roles_allowed = false,
    polar_snippet = Custom,
}

and changing the SiloUser db model to use DbTypedUuid meant that a call to to_db_typed_uuid was required. The lookup_resource macro has no type information from the string "SiloUser", so this PR adds a check: if the ancestor string is suffixed with a '*', then the lookup_resource macro should assume that the parent_id is a typed UUID, and generate the call to to_db_typed_uuid.

Most of the work after that was mechanical, changing Uuid to their typed equivalent, changing method argument types, etc etc.

Some other related things made it into this PR:

  • UserBuiltIn now also uses a typed UUID as well, distinguishing them from silo users

  • Actor no longer has the actor_id method, instead requiring call sites to check which variant of Actor is being used

  • AuthenticatedActor stores the full Actor instead of only the actor id, leading to typed comparisons in its oso::PolarClass impl

  • User and Group path params are now typed

This commit changes SiloUser and SiloGroup to use typed UUIDs.

The biggest reason that this couldn't happen without a bunch of work was
the `lookup_resource` macro: for a resource like SshKey, it looked like

```
lookup_resource! {
    name = "SshKey",
    ancestors = [ "Silo", "SiloUser" ],
    lookup_by_name = true,
    soft_deletes = true,
    primary_key_columns = [ { column_name = "id", rust_type = Uuid } ]
}
```

One of the methods that the `lookup_resource` macro generates will
create a lookup for ancestors of the resource, and this was one using
the type returned from the corresponding authz resource's `id` method:

```
            quote! { .filter(dsl::#parent_id.eq(#parent_authz_name.id())) },
```

Changing SiloUser to use a typed UUID in the authz resource:

```
authz_resource! {
    name = "SiloUser",
    parent = "Silo",
    primary_key = { uuid_kind = SiloUserKind }, <-- here
    roles_allowed = false,
    polar_snippet = Custom,
}
```

and changing the `SiloUser` db model to use `DbTypedUuid` meant that
a call to `to_db_typed_uuid` was required. The lookup_resource macro has
no type information from the string "SiloUser", so this PR adds a check:
if the ancestor string is suffixed with a '*', then the lookup_resource
macro should assume that the `parent_id` is a typed UUID, and generate
the call to `to_db_typed_uuid`.

Most of the work after that was mechanical, changing Uuid to their typed
equivalent, changing method argument types, etc etc.

Some other related things made it into this PR:

- UserBuiltIn now also uses a typed UUID as well, distinguishing them
  from silo users

- Actor no longer has the `actor_id` method, instead requiring call
  sites to check which variant of Actor is being used

- AuthenticatedActor stores the full Actor instead of only the actor id,
  leading to typed comparisons in its oso::PolarClass impl

- User and Group path params are now typed
@jmpesp jmpesp requested a review from david-crespo August 8, 2025 19:45
@david-crespo
Copy link
Contributor

Can't wait to resolve the conflicts with #7339! I just put that on auto merge, happy to handle rebasing this on top of that when it's in.

@jmpesp
Copy link
Contributor Author

jmpesp commented Aug 8, 2025

Can't wait to resolve the conflicts with #7339! I just put that on auto merge, happy to handle rebasing this on top of that when it's in.

No no, definitely land that one first! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants